Skip to content

Skip CA certificates during mTLS client certificate authentication#1279

Merged
minwoox merged 1 commit intoline:mainfrom
minwoox:mtls_leaf
Mar 24, 2026
Merged

Skip CA certificates during mTLS client certificate authentication#1279
minwoox merged 1 commit intoline:mainfrom
minwoox:mtls_leaf

Conversation

@minwoox
Copy link
Contributor

@minwoox minwoox commented Mar 19, 2026

Motivation:
The ApplicationCertificateAuthorizer previously had code to skip CA certificates commented out with a TODO note to uncomment after fixing SignedCertificateExtension to generate end-entity certificates. Now that the extension has been fixed, the filtering logic can be enabled.

Modifications:

  • Uncomment the basicConstraints check in ApplicationCertificateAuthorizer to skip CA certificates and only extract identity from leaf (end-entity) certificates

Result:

  • mTLS authentication now correctly ignores CA certificates in the peer certificate chain and only uses end-entity (leaf) certificates for application identity extraction.

@minwoox
Skip CA certificates during mTLS client certificate authentication
f24b2b6 
Motivation:
The `ApplicationCertificateAuthorizer` previously had code to skip CA
certificates commented out with a TODO note to uncomment after fixing
`SignedCertificateExtension` to generate end-entity certificates. Now that
the extension has been fixed, the filtering logic can be enabled.

Modifications:
- Uncomment the `basicConstraints` check in `ApplicationCertificateAuthorizer`
  to skip CA certificates and only extract identity from leaf (end-entity)
  certificates

Result:
- mTLS authentication now correctly ignores CA certificates in the peer
certificate chain and only uses end-entity (leaf) certificates for
application identity extraction.
@minwoox minwoox added this to the 0.81.0 milestone Mar 19, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 19, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 97921a78-1214-481e-a598-4517f8784320

📥 Commits

Reviewing files that changed from the base of the PR and between 6e561bc and f24b2b6.

📒 Files selected for processing (3)
  • server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.java
  • server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/CertificateAppIdentityAuthTest.java
  • server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java
💤 Files with no reviewable changes (1)
  • server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.java
📝 Walkthrough

Walkthrough

Uncommented the X.509 basic-constraints check in the certificate authorizer to always skip CA certificates during peer-certificate iteration. Updated two test files to pass an additional false boolean parameter to the SignedCertificateExtension constructor to accommodate the enabled check.

Changes

Cohort / File(s) Summary
Certificate Authorization
server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationCertificateAuthorizer.java		 Removed commented-out block that was conditionally disabling the CA certificate skip logic. The x509Certificate.getBasicConstraints() != -1 check now executes unconditionally during peer-certificate iteration.
Test Configuration Updates
server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/CertificateAppIdentityAuthTest.java, server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java		 Updated SignedCertificateExtension constructor calls to pass false as a third boolean parameter in client certificate setup, aligning test configuration with the now-enabled authorization logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested labels

improvement

Suggested reviewers

  • trustin
  • jrhee17
  • ikhoon

Poem

🐰 Hop, hop—commented lines now freed!
CA checks enabled, just what we need,
Tests aligned with logic so true,
X.509 paths now skip right through! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main change: uncommenting code to skip CA certificates during mTLS client certificate authentication.
Description check ✅ Passed The description is clearly related to the changeset, explaining the motivation for uncommenting the CA certificate filtering logic and the expected result.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can generate a title for your PR based on the changes.

Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the reviews.auto_title_placeholder setting.

jrhee17
jrhee17 approved these changes Mar 20, 2026
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

ikhoon
ikhoon approved these changes Mar 24, 2026
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

@minwoox minwoox merged commit e8ba7ca into line:main Mar 24, 2026
14 checks passed
@minwoox minwoox deleted the mtls_leaf branch March 24, 2026 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ikhoon ikhoon ikhoon approved these changes

@jrhee17 jrhee17 jrhee17 approved these changes

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees

No one assigned

Labels

cleanup

Projects

None yet

Milestone

0.81.0

Development

Successfully merging this pull request may close these issues.

3 participants

@minwoox @ikhoon @jrhee17